Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing static API #107

Merged
merged 2 commits into from
May 13, 2017
Merged

Removing static API #107

merged 2 commits into from
May 13, 2017

Conversation

nbarbettini
Copy link
Contributor

Rebased #103

@nbarbettini nbarbettini requested a review from abatishchev May 13, 2017 18:13
@nbarbettini nbarbettini mentioned this pull request May 13, 2017
@nbarbettini
Copy link
Contributor Author

nbarbettini commented May 13, 2017

@abatishchev I noticed you removed these files:

These tests aren't duplicated anywhere else. I think maybe we should change them to using the instance (not static), instead of deleting them?

@abatishchev
Copy link
Member

So did I :-D Never mind, closed that PR in favor of this one.

Regarding the tests. IIRC, new tests are just clones of old tests except the part where static factory is assigned to substitute JSON (de)serializer. So we should be fine by deleting them. An additional review by separate pair of eyes would be helpful though!

@abatishchev abatishchev added this to the 3.0 milestone May 13, 2017
@abatishchev abatishchev reopened this May 13, 2017
@abatishchev
Copy link
Member

Oops, closed wrong one. Reopened.

@nbarbettini
Copy link
Contributor Author

Re: tests - I agree that the duplicate tests in JWT.Tests.Core are unnecessary and can be deleted. However, the tests in JWT.Tests.NETFramework were testing the other deserializer plugins and may still be useful. Do you want to keep the System.Web and ServiceStack tests around?

@abatishchev
Copy link
Member

abatishchev commented May 13, 2017

Frankly I don't. This functionality goes away with the removal of static API.
To add it back and this time to do this properly, there should be 2 additional/separate nuget packages (as per #76) with their own tests. Those might be just clones of standard tests but targeted against their own implementation.
Makes sense?

@nbarbettini
Copy link
Contributor Author

Ah, now I'm understanding you. Yep! Let's merge.

@abatishchev abatishchev merged commit fc81946 into master May 13, 2017
@abatishchev abatishchev deleted the no-static-api-rebased branch May 13, 2017 18:33
@abatishchev
Copy link
Member

Pushed to NuGet as 3.0.0-beta3.

@nbarbettini Thanks for your help! As always :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants